-
Notifications
You must be signed in to change notification settings - Fork 140
Make Number non-exhaustive #564
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@epage What are your thoughts on this compromise? |
| #[cfg(not(doc))] | ||
| #[allow(private_interfaces)] | ||
| __NonExhaustive(private::Never), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure whats going on but this didn't make the enum exhaustive. I wonder if the compiler recognized that this variant cannot be constructed so therefore it doesn't need to be matched?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uff, maybe this comes from some of the newer uninhabited type matching ...
Since private::Never is private, we can make the type inhabited but not constructible. Would changing this be a breaking change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, people can exhaustively match on Number. Making it actually non-exhaustive would then be a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We’ll definitely need a compile-fail test then to ensure there are no more slip ups - I’m sorry that the first fix didn’t catch this.
Fixes #563
CHANGELOG.md